[#1068] Apply what a running backend takes of a configuration change, and ask for a restart for what it does not - #1069
Conversation
48a7d9e to
4bc401f
Compare
|
Rebased onto the restacked #1066 ( Re-run green on the rebased head: |
4bc401f to
4ea1d18
Compare
|
Restacked on the new head of #1066 ( |
4ea1d18 to
d2da8dd
Compare
|
Restacked on the new head of #1066 ( No change of its own to re-test either; |
maximthomas
left a comment
There was a problem hiding this comment.
praise: The change goes where #1068 says the gap is and reports through the channel that reaches the log.
JEStorage.applyToEnvironment(JEStorage.java:1441-1465) is the firstEnvironment.setMutableConfigcall insrc/main, and the six mapped properties plus the durability every way reach the running environment (aChangeOfWhatJETakesWhileItRunsReachesTheEnvironment,aChangeOfTheDurabilityReachesTheEnvironmentEveryWay).- Every differing immutable parameter goes into the change result with
NOTE_CONFIG_DB_PROPERTY_REQUIRES_RESTART(631) andadminActionRequired, not only into the XML marking, and the both-flags durability is now refused byisConfigurationChangeAcceptable(checkEnvironmentConfiguration,JEStorage.java:1337) instead of failing the next open. - Green on CI at this head:
JEStorageTest17/17 andPDBStorageTest22/22 on build-maven (ubuntu-latest, 11).
issue (blocking): Removing a mutable je-property leaves the running environment on the old value, and the change result reports it applied.
opendj-server-legacy/src/main/java/org/opends/server/backends/jeb/JEStorage.java:1449, :1465; opendj-maven-plugin/src/main/resources/config/xml/org/forgerock/opendj/server/config/JEBackendConfiguration.xml:733
applyToEnvironment builds next from cfg and hands it to env.setMutableConfig(next), and the loop skips every mutable parameter. In JE 18.3.12, EnvironmentImpl.doSetMutableConfig clones the running config and EnvironmentMutableConfig.copyMutablePropsTo copies only the names next sets explicitly (its Properties has no defaults). So after dsconfig set-backend-prop --remove je-property:je.cleaner.minAge=5, next no longer sets je.cleaner.minAge, and the environment runs with 5 until the next open. The change result carries no message and no admin action. That is the "reported as applied, runs on unchanged" outcome #1068 removes, on a road the description claims ("a mutable je-property is applied") and the new je-property synopsis documents ("... and the change result says so"). Replacing a value is applied, and removing an immutable one is reported: getConfigParam resolves an unset name to JE's default, so the diff sees it. Only the mutable removal is dropped. Mapped properties and durability are always set by toEnvironmentConfig and are not affected.
for (ConfigParam param : new TreeMap<>(EnvironmentParams.SUPPORTED_PARAMS).values())
{
// Replication parameters are not set through an environment configuration; a multi-value
// parameter is not read as one value. Neither is set by this storage.
if (param.isForReplication() || param.isMultiValueParam())
{
continue;
}
final String runningValue = running.getConfigParam(param.getName());
final String nextValue = next.getConfigParam(param.getName());
if (Objects.equals(runningValue, nextValue))
{
continue;
}
if (param.isMutable())
{
// setMutableConfig copies only what the configuration handed to it sets: a parameter the
// configuration no longer sets goes back to JE's default rather than keeping its value.
next.setConfigParam(param.getName(), nextValue);
}
else
{
ccr.setAdminActionRequired(true);
ccr.addMessage(NOTE_CONFIG_DB_PROPERTY_REQUIRES_RESTART.get(
ConfigurableEnvironment.configuredNameOf(param.getName()), cfg.getBackendId(), runningValue, nextValue));
}
}Pin (JEStorageTest, red at this head): apply a cfg whose getJEProperty() is new TreeSet<>(Arrays.asList(CLEANER_MIN_AGE + "=5")), assert env.getMutableConfig().getConfigParam(CLEANER_MIN_AGE) is "5", then apply createBackendCfg() and assert getMessages() is empty and the parameter is back to the value read before the first change.
issue (non-blocking): After an open the quota refused, every modify of the backend entry is refused. This is #1066's blocking item, carried here by the stacked commit.
opendj-server-legacy/src/main/java/org/opends/server/backends/jeb/JEStorage.java:1289, opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java:1557
(newSize <= reservedCacheSize || quota.isMemoryAvailable(newSize - reservedCacheSize)) with reservedCacheSize == 0 refuses a modify that leaves the cache alone. That includes the db-checkpointer-wakeup-interval change this PR reports on, and TaskUtils.disableBackend for an online import, rebuild or restore. It comes from bd9d9d1 (#1066) and d2da8dd leaves it alone; the new checks only follow the &&. It is blocking on #1066, not twice. Rebasing onto #1066's fix clears it here.
issue (non-blocking): A modify that also moves db-directory returns before applyToEnvironment and the checkpoint arm run.
opendj-server-legacy/src/main/java/org/opends/server/backends/jeb/JEStorage.java:1393, :1416; opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java:1639, :1660
The permissions block is entered whenever the directory differs, and if (!ccr.getMessages().isEmpty()) return ccr; then returns because NOTE_CONFIG_DB_DIR_REQUIRES_RESTART is already in the result, not because of an error. A modify of db-directory plus db-txn-no-sync, a je-property, or the PDB interval therefore gets the directory note only: the durability is not applied, no 631 names the immutable change, and config = cfg is skipped, so every later modify before the restart takes the same return. The return predates this PR, but the new application and reporting sit below it. The directory note already asks for a restart, which limits the harm. StorageUtils.addErrorMessage sets the result code, so test that instead of whether the result has messages, in both storages (with import org.forgerock.opendj.ldap.ResultCode;):
if (ccr.getResultCode() != ResultCode.SUCCESS)
{
return ccr;
}suggestion (non-blocking): No case changes db-cache-percent while the storage is open, so nothing pins the MAX_MEMORY_PERCENT copy.
opendj-server-legacy/src/main/java/org/opends/server/backends/jeb/JEStorage.java:1463; opendj-server-legacy/src/test/java/org/opends/server/backends/jeb/JEStorageTest.java:492, :655
Every test cfg stubs getDBCachePercent() to 20, and the one cache case (aChangeWhileOpenLeavesTheCacheWhereTheOpenReservedIt) changes db-cache-size on a SMALL_CACHE environment. The copy is therefore a no-op in every case, and deleting :1463 survives the file. On a percent-sized backend (db-cache-size 0, the default), dropping the copy would let a percent change resize the live cache while the quota still holds the open's reservation.
@Test
public void aCachePercentChangedWhileOpenLeavesTheCacheWhereTheOpenReservedIt() throws Exception
{
final Environment env = environmentOf(storage);
final long cacheAtOpen = env.getMutableConfig().getCacheSize();
final JEBackendCfg cfg = createBackendCfg();
when(cfg.getDBCachePercent()).thenReturn(30);
assertThat(storage.applyConfigurationChange(cfg).adminActionRequired()).isTrue();
assertThat(env.getMutableConfig().getCacheSize()).isEqualTo(cacheAtOpen);
}Pin: red with :1463 deleted.
suggestion (non-blocking): No test pins that the PDB checkpoint arm compares against the interval the database runs with rather than against the previous configuration.
opendj-server-legacy/src/test/java/org/opends/server/backends/pdb/PDBStorageTest.java:716; opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java:1660
Both interval cases make one change from the open state, where config's interval equals the database's. The mutant cfg.getDBCheckpointerWakeupInterval() != config.getDBCheckpointerWakeupInterval(), with the from-value read off config, gives the same text on the open case and stays silent on the closed one. Under it, a pending restart stops being reported after an unrelated change. Append to aCheckpointIntervalChangedWhileOpenAsksForARestart:
// a later change which leaves the interval where the first one put it still asks for the restart
assertThat(storage.applyConfigurationChange(cfg).adminActionRequired()).isTrue();
// one which puts it back to what the database runs asks for nothing
assertThat(storage.applyConfigurationChange(createBackendCfg()).getMessages()).isEmpty();Pin: both lines go red under the mutant.
d2da8dd to
871db80
Compare
|
Round head: Blocking: a removed mutable
So
Non-blocking: every modify refused after a refused open. This is fixed by #1066's round and carried here by the restack: Non-blocking: a moved
Suggestion: the percent pin. Taken as Suggestion: the checkpoint interval across two changes. Your two lines are appended to Runs. Reactor
Each mutant is red on exactly the cases listed and green elsewhere. |
maximthomas
left a comment
There was a problem hiding this comment.
praise: 871db80 closes all three round-1 findings where they arise.
JEStorage.resetsToDefault(JEStorage.java:1662) puts JE's default back for a removed mutableje-property, and reports 631 wheresetConfigParamrefuses that default.- The permission gates now return on the result code (
JEStorage.java:1580,:1586,PDBStorage.java:1646,:1652), so the moved-directory note no longer drops the rest of the change. - The stacked cba9d51 admits a size up to
Math.max(reservedCacheSize, computeSize(config))(JEStorage.java:1473,PDBStorage.java:1561).
issue (blocking): After an online db-directory change, getDirectory() names the new, empty directory while the environment keeps running on the old one.
opendj-server-legacy/src/main/java/org/opends/server/backends/jeb/JEStorage.java:1608, :1212-1215, :1224-1227; opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java:1677, :1338-1341
The note for a moved directory sets no error code, so the change now reaches config = cfg (:1608). getDirectory() is getBackendDirectory(config), but the environment was opened on the final backendDirectory (:995). The new directory is also empty, because checkDBDirExistsOrCanCreate(newDir, ccr, false) (:1564) creates it and never removes it. BackupTask takes only a shared lock, so a backup taken before the restart runs JELogFilesIterator(getDirectory()) over the empty directory. BackupManager.createBackup (BackupManager.java:1234-1247) finds no file, writes an empty placeholder, and reports success.
There is a second effect. close() (:907) deregisters getDirectory(), which is now the new directory, but the disk-monitor registrations are keyed by the old one (DiskSpaceMonitor.java:310-322). The closed storage therefore stays registered as a handler for the old directory.
PDB changes config the same way (:1677). Its online listing returns Persistit's paths under the old directory, and those are then made relative to the new root. I did not trace the PDB half further. Round 1 suggested the result-code gate, and that suggestion missed that getDirectory() reads config.
@Override
public File getDirectory()
{
// The directory the storage runs on; a moved db-directory is used from the next open.
return backendDirectory;
}PDBStorage.getDirectory() gets the same change. Both storages build backendDirectory from the same cfg as config at construction, so restore and offline backup, which use a fresh storage, are unchanged.
Pin: in aChangeWhichMovesTheDirectoryIsStillAppliedToTheEnvironment (JEStorageTest.java:662), read final File before = storage.getDirectory(); before the apply. After it, assert assertThat(storage.getDirectory()).isEqualTo(before); assertThat(storage.getFilesToBackup().hasNext()).isTrue();. This is red at 871db80 and green with the fix. Add the same getDirectory() assertion to PDBStorageTest.aChangeWhichMovesTheDirectoryStillReportsTheRest (:863). Note that asserting getDirectory() equals the moved directory would pin the defect.
question (non-blocking): Is a je.properties file in the backend's db directory a supported way to tune a JE backend? applyToEnvironment does not see it.
opendj-server-legacy/src/main/java/org/opends/server/backends/jeb/JEStorage.java:1642, :1647-1649, :1662
At the open, JE applies <env home>/je.properties over the config it is handed (JE Environment.java:312-315; the load is on by default). setMutableConfig never reads the file again, so running holds the file's values and toEnvironmentConfig(cfg) does not. A mutable value set only by the file (e.g. je.checkpointer.highPriority=true) is put back to JE's default by resetsToDefault on the next online change of any property, with no message. This is new in 871db80. An immutable value from the file (e.g. je.log.fileMax) gets a 631 and adminActionRequired on every change, and the restart the note asks for applies the file again, so the note never clears. This is a Minor if the file is unsupported. If it is supported, the silent reset is the "reported as applied, runs on something else" outcome #1068 removes, and it becomes a Major.
// As the open does: je.properties in the environment home overrides the configuration.
final File propertyFile = new File(backendDirectory, "je.properties");
if (propertyFile.isFile())
{
final Properties fileProperties = new Properties();
try (InputStream in = new FileInputStream(propertyFile))
{
fileProperties.load(in);
}
for (String name : fileProperties.stringPropertyNames())
{
if (name.startsWith("je."))
{
next.setConfigParam(name, fileProperties.getProperty(name));
}
}
}This goes right after next is built in applyToEnvironment, which then declares IOException; applyConfigurationChange already catches Exception. Or: state in the je-property synopsis that an online change resets what je.properties sets.
suggestion (non-blocking): No test takes the error arm of the four new result-code gates.
opendj-server-legacy/src/main/java/org/opends/server/backends/jeb/JEStorage.java:1580, :1586; opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java:1646, :1652
The move cases kill a revert to "return on any message", but every getDBDirectoryPermissions stub in the test tree returns "755". An if (false) mutant on any of the four gates therefore survives both classes. Under that mutant, a refused mode would be written to the running directory and the rest of the change applied on an error result.
final Environment env = environmentOf(storage);
final Durability before = env.getConfig().getDurability();
final JEBackendCfg insaneMode = createBackendCfg();
when(insaneMode.getDBDirectoryPermissions()).thenReturn("500");
when(insaneMode.isDBTxnNoSync()).thenReturn(true);
when(insaneMode.isDBTxnWriteNoSync()).thenReturn(false);
final ConfigChangeResult ccr = storage.applyConfigurationChange(insaneMode);
assertThat(ccr.getResultCode()).isNotEqualTo(ResultCode.SUCCESS);
assertThat(env.getConfig().getDurability()).isEqualTo(before);Pin: this kills the :1580 mutant. The PDB twin at :1646 needs the same case with a changed db-txn-no-sync. The gates at :1586 and :1652 need setPermissions to fail, for example on a read-only parent.
suggestion (non-blocking): isConfigurationAcceptable is tested only with the durability conflict, and only by counting reasons. The unknown native property never reaches it.
opendj-server-legacy/src/test/java/org/opends/server/backends/jeb/JEStorageTest.java:810-811
The Tests paragraph says both inputs "are refused by both acceptability checks". unknownProperty only goes to isConfigurationChangeAcceptable, and hasSize(1) would also pass if the refusal had a different cause. A mutant that replaces the checkEnvironmentConfiguration call in isConfigurationAcceptable (JEStorage.java:1513) with a durability-only check survives this case. I did not settle whether the quota arm can fire for createBackendCfg().
assertThat(reasons.get(0).toString()).isEqualTo(ERR_CONFIG_JEB_DURABILITY_CONFLICT.get().toString());
reasons.clear();
assertThat(JEStorage.isConfigurationAcceptable(unknownProperty, reasons, serverContext)).isFalse();
assertThat(reasons).hasSize(1);
assertThat(reasons.get(0).ordinal()).isEqualTo(ERR_CONFIG_JE_PROPERTY_INVALID.get("", "").ordinal());…onfiguration change, and ask for a restart for what it does not Nine properties of the JE and PDB backends were neither applied to a running backend nor marked as requiring a restart. JEStorage.applyConfigurationChange handled the directory, its permissions and the disk thresholds and left the environment - configured once, at the open - as it was, while the XML kept db-cleaner-min-utilization, db-run-cleaner, db-evictor-core-threads, db-evictor-max-threads, db-evictor-keep-alive, db-num-cleaner-threads, db-txn-no-sync and db-txn-write-no-sync (JE) and db-checkpointer-wakeup-interval (PDB) as live properties, which they had been in the local-db backend OPENDJ-1719 replaced. A change of any of them was reported as applied while the backend ran on unchanged until it was next opened; so was a native property changed through je-property. JEStorage now builds the environment configuration the changed configuration describes and hands it to Environment.setMutableConfig, which takes of it what JE accepts while it runs: the properties above, the durability, and a mutable native property - all but the cache, which stays with the memory reserved for it until the restart OpenIdentityPlatform#1063 asks for. Every immutable JE parameter whose value differs from the running environment's is reported with the new NOTE 631, which names the property, the value the environment runs with and the one configured, and reaches the error log as a warning - where the change result of a property marked in the XML alone never did. An import's environment is left alone: it runs on a configuration of its own, and the backend opens again on the changed one once the import is over. The build of the environment configuration is split from the checks of the open (ConfigurableEnvironment.toEnvironmentConfig): no cache size probe against the memory quota, no level set on the JE loggers, so that a configuration change can be checked against it as well - and it is: isConfigurationChangeAcceptable and isConfigurationAcceptable refuse a durability which sets both flags (db-txn-write-no-sync is on by default, so setting db-txn-no-sync alone is one) and a native property JE does not know before the change is written. Nothing checked either before, and the backend failed to open on them at its next restart. A configuration which sets neither durability flag now sets COMMIT_SYNC explicitly: what JE falls back on, but set, since JE leaves the durability an environment has in place when a configuration hands it none. PDBStorage reports a change of db-checkpointer-wakeup-interval with the same note, holding the configured interval against the one the database opened with - PersistIt takes no configuration once one is set - and the property is marked component-restart in PDBBackendConfiguration.xml. The definition of je-property says which of its changes wait for a restart.
…s default, and apply the rest of a change which moves the directory A mutable je-property removed from the configuration stayed in the running environment: setMutableConfig copies only what the configuration handed to it sets, onto a clone of the running one. applyToEnvironment now hands JE's default explicitly for a mutable parameter the configuration no longer sets, and asks for a restart (631) where JE does not take its own default as a value - je.cleaner.readSize, whose 0 means "computed at the open", is refused below 128. The two checks after the directory permissions returned on any message, and the note of a moved db-directory is one: a change which moved the directory as well skipped the environment, the checkpoint interval, the cache note and config = cfg. Both storages now return on the result code StorageUtils.addErrorMessage sets. Tests: the removal applied, the removal JE cannot take while it runs, a moved directory with the rest of the change (JE and PDB), a percentage change which leaves the live cache alone, and the checkpoint interval held against the database across two changes.
…til the restart, and hold a moved directory against it Once a change which moves db-directory went on to config = cfg, getDirectory() - read from the configuration - named the new, empty directory while the environment ran on the one it was opened on: an online backup listed no file and reported success, and close() deregistered the new directory from the disk monitor instead of the one registered. Both storages now return the directory they were built on, which a new storage - the next open of the backend - builds from the moved configuration. The move was also compared with the configuration as last changed, so only the first change asked for the restart. Both storages now compare the new directory with the one they run on: a later change asks for the restart again, one which moves back asks for nothing, and the note names the backend's directories rather than the db-directory values. Tests: a moved directory keeps the directory the storage runs on and a non-empty backup list, and asks again on a later change and for nothing when moved back (JE and PDB); a directory mode the server could not use refuses the change whole (JE and PDB); the static acceptability check refuses an unknown native property for that reason, and the durability conflict for its own.
871db80 to
a5cf106
Compare
|
Round head: Blocking: That fix leaves one more gap, which I closed in the same commit.
Question:
Suggestion: the error arm of the result-code gates. Taken for the permissions check: The gate after Suggestion: the static acceptability check. Taken. Runs. Reactor
Each mutant is red on exactly the case listed and green elsewhere. |
maximthomas
left a comment
There was a problem hiding this comment.
praise: Round 2's blocker is fixed where it lives, and the fix is pinned in both storages.
JEStorage.getDirectory()(:1214) andPDBStorage.getDirectory()(:1338) now returnbackendDirectory. After a move,getFilesToBackup()and the deregister inclose()name the directory the environment runs on.moved = !newBackendDirectory.equals(backendDirectory)(JEStorage.java:1566,PDBStorage.java:1630) asks again for a pending restart on every later change, and asks nothing on a move back. The repeat and the move back are asserted in both move tests.aChangeToAnInsaneDirectoryModeIsRefusedWhole(JE and PDB) kills the mutant that deletes the first permission gate.
issue (blocking): A change that switches je.maxOffHeapMemory between zero and non-zero fails. After it, every later change of the backend entry also fails until the backend restarts.
opendj-server-legacy/src/main/java/org/opends/server/backends/jeb/JEStorage.java:1662, :1649-1651, :1617-1619
applyToEnvironment passes every parameter JE marks mutable to env.setMutableConfig(next): values the configuration sets, and defaults put back by resetsToDefault. je.maxOffHeapMemory is mutable in JE 18.3.12 (EnvironmentParams.MAX_OFF_HEAP_MEMORY). But OffHeapCache.envConfigUpdate throws IllegalArgumentException("Cannot change off-heap cache size between zero and non-zero"), and by then EnvironmentImpl.doSetMutableConfig has already swapped its config manager to the new value. The first change is admitted (ConfigurableEnvironment.setJEProperties accepts it). It then ends in an error result carrying a stack trace, and config = cfg is skipped. From then on env.getConfig() reports the new value while OffHeapCache still holds the old one. Every later setMutableConfig throws the same exception, so any change of the backend entry fails, db-cache-percent or a durability flag included. A single-file probe against je-18.3.12.jar on JDK 26 shows this in both directions: adding the property to an environment opened with 0, and removing it from one opened with N. In both cases an unrelated later change throws too. On master applyConfigurationChange never calls setMutableConfig, so both changes succeed and take effect at the next open. Of the 13 JE EnvConfigObservers, this is the only one that refuses a value.
if (Objects.equals(runningValue, nextValue)
|| (param.isMutable()
&& !flipsOffHeapCache(param.getName(), runningValue, nextValue)
&& (next.isConfigParamSet(param.getName())
|| resetsToDefault(next, param.getName(), nextValue))))
{
continue;
}
// ... 631 as before
}
next.setConfigParam(MAX_MEMORY, running.getConfigParam(MAX_MEMORY));
next.setConfigParam(MAX_MEMORY_PERCENT, running.getConfigParam(MAX_MEMORY_PERCENT));
// JE refuses to switch its off-heap cache on or off while the environment runs: the next open does.
if (flipsOffHeapCache(MAX_OFF_HEAP_MEMORY,
running.getConfigParam(MAX_OFF_HEAP_MEMORY), next.getConfigParam(MAX_OFF_HEAP_MEMORY)))
{
next.setConfigParam(MAX_OFF_HEAP_MEMORY, running.getConfigParam(MAX_OFF_HEAP_MEMORY));
}
env.setMutableConfig(next);
}
private static boolean flipsOffHeapCache(String name, String runningValue, String nextValue)
{
return MAX_OFF_HEAP_MEMORY.equals(name)
&& (Long.parseLong(runningValue) > 0) != (Long.parseLong(nextValue) > 0);
}Pin: open on createBackendCfg(), then apply a cfg whose getJEProperty() returns new TreeSet<>(Arrays.asList("je.maxOffHeapMemory=1048576")). Assert SUCCESS and exactly one message, NOTE_CONFIG_DB_PROPERTY_REQUIRES_RESTART by ordinal. Then apply createBackendCfg() again and assert SUCCESS with no message. Both asserts fail at a5cf106 (error result) and pass with the fix. This is the add road, so the open allocates no off-heap memory.
issue (non-blocking): If a change moves db-directory out with a new db-directory-permissions and a later change moves it back, the running directory never gets the new mode.
opendj-server-legacy/src/main/java/org/opends/server/backends/jeb/JEStorage.java:1581, opendj-server-legacy/src/main/java/org/opends/server/backends/pdb/PDBStorage.java:1645
The move is now compared with backendDirectory, but the mode is still compared with config. The move out has already set config to the new mode, although it chmodded only the new directory. Example: running on D with mode 755. Change 1 sets db-directory X with mode 700: X is chmodded and the restart note is added. Change 2 sets db-directory back to D, still with mode 700. Nothing has moved and the mode equals config's, so D is not chmodded, no note is added, and the result is SUCCESS. D stays at 755, while the configuration says 700, until the next open re-applies the mode (StorageUtils.setupStorageFiles). On master and at 871db80 change 2 chmodded D.
if (moved || !cfg.getDBDirectoryPermissions().equalsIgnoreCase(runningDirectoryPermissions))
{
// ... checkDBDirPermissions / setDBDirPermissions(newBackendDirectory, ...) as now
if (!moved)
{
runningDirectoryPermissions = cfg.getDBDirectoryPermissions();
}
}runningDirectoryPermissions is a new field, set where the open applies the mode. Pin: in aChangeWhichMovesTheDirectoryIsStillAppliedToTheEnvironment, give the move out getDBDirectoryPermissions() "700" (the fixture opens with "755"), move back with "700", and assert Files.getPosixFilePermissions(directoryAtOpen.toPath()) equals PosixFilePermissions.fromString("rwx------"). It fails at a5cf106 (rwxr-xr-x). Do the same in the PDB move test.
suggestion (non-blocking): No case pins that close() deregisters the directory that registerMonitoredDirectory registered after a move.
opendj-server-legacy/src/test/java/org/opends/server/backends/jeb/JEStorageTest.java:710-743, opendj-server-legacy/src/test/java/org/opends/server/backends/pdb/PDBStorageTest.java:912-938
The commit message counts the deregister among the fixes. Today both calls go through getDirectory(), which the move tests pin. But the fixture hands the storage an anonymous mock(DiskSpaceMonitor.class), so no case checks what was registered. Suppose a mutant makes registerMonitoredDirectory register getBackendDirectory(cfg). Both move tests still pass.
final DiskSpaceMonitor monitor = serverContext.getDiskSpaceMonitor();
// ... after the three applies:
verify(monitor, never()).registerMonitoredDirectory(
anyString(), argThat(directory -> !directory.equals(directoryAtOpen)), anyLong(), anyLong(), any());Pin: the verify above kills that mutant. Add the same check to aChangeWhichMovesTheDirectoryStillReportsTheRest.
Fixes #1068. On master
1aa253d7f6(#1066, which this was stacked on, is merged): it changes the same lines of bothapplyConfigurationChange, and 631 follows #1066's 630.What was wrong
Nine properties of the JE and PDB backends were neither applied to a running backend nor marked as requiring a restart, so
dsconfigreported a change of them applied and the backend ran on unchanged until it was next opened.JEStorage.applyConfigurationChangehandleddb-directory, its permissions and the disk thresholds and left the environment - configured once, at the open - as it was; nothing in the server calledEnvironment.setMutableConfig. The same went for a native property changed throughje-property, mutable or not (not in the issue's table, same cause).Two things the issue had wrong, found on the way:
requires-admin-actionin the XML reaches the reference documentation and property help alone -dsconfigprints nothing about it at set time, and a change result withoutadminActionRequiredcarries nothing to the error log either. So "asdb-log-file-maxalready does" was not true of the change result: the marking is all those properties had. The oldRootContainerreported every changed immutable parameter in the change result, which the server logs as a warning (WARN 647); that shape is restored for all of them.db-txn-write-no-syncis on by default, sodsconfig set-backend-prop --set db-txn-no-sync:trueon a JE backend yields a durability which sets both flags. Nothing checked that at change time: the change was admitted and written, and the backend failed to open onERR_CONFIG_JEB_DURABILITY_CONFLICTat its next restart. (aChangeWhichLeavesTheCacheSizeAloneAsksForNothingof [#1063] Give back what the open reserved rather than what the configuration says by then, and ask for a restart when the cache size changes #1066 used exactly that as its "unrelated change"; it now sets the write flag off as well.)What this does
JE -
applyToEnvironmentbuilds the environment configuration the changed configuration describes (ConfigurableEnvironment.toEnvironmentConfig) and hands it toEnvironment.setMutableConfig, which takes of it what JE accepts while it runs:db-cleaner-min-utilization,db-run-cleaner,db-evictor-core-threads,db-evictor-max-threads,db-evictor-keep-alive,db-num-cleaner-threads, the durability (db-txn-no-sync/db-txn-write-no-sync, every way - a configuration which sets neither now setsCOMMIT_SYNCexplicitly, since JE leaves the durability an environment has in place when handed none) and a mutableje-property- removed as well as set: the environment keeps the value it runs with of a parameter it is not handed, so a mutable parameter the configuration no longer sets is handed JE's default explicitly, and one whose default JE does not take as a value (je.cleaner.readSize, whose0stands for "computed at the open") asks for a restart with 631 instead. All but the cache:je.maxMemory/je.maxMemoryPercentare mutable too, but the cache stays with the memory reserved for it until the restart #1063 asks for, so the change hands the environment its current values back. Every immutable JE parameter whose value differs from the running environment's is reported with the newNOTE_CONFIG_DB_PROPERTY_REQUIRES_RESTART(631), naming the property asdsconfigknows it (or the JE property name forje-property), the value the environment runs with and the one configured. An import's environment is left alone - it runs on a configuration of its own and the backend opens again on the changed one once the import is over; held to the import's configuration, every property the import sets differently would ask for a restart.A change which moves
db-directoryas well is still applied and reported: both storages returned on any message after the permission checks, and the note of the moved directory is one - they now return on the result code an error sets. The configuration then moves on while the storage runs on the directory it was opened on until the restart, sogetDirectory()- which the backup lists andclose()deregisters from the disk monitor - returns that directory rather than reading the configuration, and a move is held against it: a later change still asks for the restart, one which moves back asks for nothing, and the note names the backend's directories (<db-directory>/<backend-id>).toEnvironmentConfigis the build alone - no cache size probe against the memory quota (#1067), no level set on the JE loggers (that moves toparseConfigEntry, the open's road) - so that a change can be checked against it:isConfigurationChangeAcceptableandisConfigurationAcceptablenow refuse a conflicting durability and a native property JE does not know before the change is written.PDB -
db-checkpointer-wakeup-intervalis set on the PersistIt configuration at the open alone andPersistit.setConfigurationrefuses once one is set, so a change of it reports 631 against the interval the database opened with (db.getConfiguration().getCheckpointInterval(), no new field), and the property is markedcomponent-restartinPDBBackendConfiguration.xml.je-property's definition says which of its changes wait for a restart.Left as they are:
db-logging-levelanddb-logging-file-handler-onkeep theircomponent-restartmarking. The JUL level is set by the open alone, as before; the file handler's level (je.env.fileLoggingLevel) is mutable in JE and so follows a change from now on - the marking is conservative about it, not wrong.Tests
JEStorageTest: the six mapped properties reachenv.getMutableConfig(); the durability follows the change every way; a mutableje-propertyis applied and an immutable one asks for a restart with 631; a removed mutable one goes back to JE's default, or asks for a restart where JE refuses its default; a change which moves the directory is still applied, keeps the directory the environment runs on with a non-empty backup list, asks again on a later change and for nothing when moved back; a directory mode the server could not use refuses the change whole; a change ofdb-cache-percentleaves the live cache alone;db-log-file-maxasks for a restart with 631; a change while open leaves the cache where the open reserved it (the #1063 interplay); a change during an import leaves the import's environment alone; a change while closed touches nothing; a durability which sets both flags and an unknown native property are refused by both acceptability checks, each for its own reason.PDBStorageTest: a changed interval asks for a restart with 631 and the database keeps its own, held against the database across two changes; a change which moves the directory still reports the interval, keeps the directory the database runs on, asks again on a later change and for nothing when moved back; a directory mode the server could not use refuses the change whole; a change while closed asks for nothing.Verified locally: the issue reproduced on master, on the #999 head and on the #1066 head with a direct TestNG repro of every row (JE 7/7 red, PDB 1/1 red,
db-txn-no-syncon PDB green as a control); the new tests are red without the fix (5 JE + 1 PDB) and green with it; mutants (nosetMutableConfig, cache not pinned, immutables unreported, import's environment held to the configuration, PDB silent) each red on their own case; regression set of 18 classes / 240 tests (FailedBackendOpenTest, PDB/JE TestCase + Encrypted, ReplayedConfigChangeTest, OnDiskMergeImporterTest, the pluggable tree tests, ImportLDIF/RebuildIndex/VerifyIndex, BackendConfigManagerTestCase) green.Review rounds, mutants each red on exactly its own case: round 1 - mutable parameters skipped, the reviewer's reset without the fallback, a return on any message (JE, PDB), the percent copy deleted, the PDB interval against
config; round 2 -getDirectory()fromconfig(JE, PDB), the move againstconfig(JE, PDB), the permissions gate dropped (JE, PDB), the static check without the environment build. At the round 2 headJEStorageTest37/37 andPDBStorageTest31/31.